Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not serve WebP images that are larger than the original #6123

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Roy-Orbison
Copy link

The current code assumes the .webp is always smaller, but occasionally it isn't. Optimiser plugins may report this situation as "already optimised", or similar. In some cases, the optimiser plugin provides both a WebP version and an optimised version with the same name as the original, and the WebP might be smaller than the original but larger than the other optimised file.

Description

This adds a size check to the webp URL replacer function. I noticed that Rocket was still replacing URLs for which Imagify reports "already optimised", so the images served were larger, which degraded performance.

Type of change

  • Bug fix (non-breaking change which fixes an issue).
  • This change requires a documentation update. (I was thinking a note somewhere on https://docs.wp-rocket.me/article/1282-webp tha mentions webp would not be served unless it is smaller than the main file.)

Is the solution different from the one proposed during the grooming?

N.a.

Checklists

Generic development checklist

  • My code follows the style guidelines of this project, with adapted comments and without new warnings.
  • I have added unit and integration tests that prove my fix is effective or that my feature works.
  • The CI passes locally with my changes (including unit tests, integration tests, linter).
  • Any dependent changes have been merged and published in downstream modules.
  • If applicable, I have made corresponding changes to the documentation.

Test summary

  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I validated all Acceptance Criteria of the related issues. (If applicable, provide proof).
  • I validated all test plan the QA Review asked me to.

I ran this patch on a live site (with an additional logging line inside the size check if block), and it worked as intended.

The current code assumes the .webp is always smaller, but occasionally
it isn't. Optimiser plugins may report this as "already optimised" or
similar.
@@ -435,14 +435,20 @@ private function url_to_webp( $url, $extensions ) {

if ( $this->filesystem->exists( $src_path_webp ) ) {
// File name: image.jpg => image.webp.
return preg_replace( '@\.' . $src_url['extension'] . '$@', '.webp', $src_url['src'] ) . $src_url['query'];
$src_url_webp = preg_replace( '@\.' . $src_url['extension'] . '$@', '.webp', $src_url['src'] ) . $src_url['query'];
} elseif ( $this->filesystem->exists( $src_path_webp = $src_path . '.webp' ) ) { // phpcs:ignore Generic.Sniffs.CodeAnalysis.AssignmentInCondition,Squiz.Sniffs.PHP.DisallowMultipleAssignments
Copy link
Author

@Roy-Orbison Roy-Orbison Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assigning within the condition avoids unnecessarily nested if statements. it's not meant to be a comparison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant